Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src,lib: rename FSReqWrap to FSReqCallback #21971

Closed
wants to merge 2 commits into from
Closed

src,lib: rename FSReqWrap to FSReqCallback #21971

wants to merge 2 commits into from

Conversation

maclover7
Copy link
Contributor

Given that FSReqPromise does not inherit from FSReqWrap, FSReqWrap
should be renamed FSReqCallback to better describe what it does.

First of a few upcoming fs refactorings :)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@maclover7 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/347/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 25, 2018
@maclover7 maclover7 added the fs Issues and PRs related to the fs subsystem / file system. label Jul 26, 2018
@maclover7
Copy link
Contributor Author

Full CI: https://ci.nodejs.org/job/node-test-pull-request/16012/

@joyeecheung
Copy link
Member

joyeecheung commented Jul 26, 2018 •
edited
Loading

The change to the binding and to the resource types are potentially breaking so this may be semver-major.

cc @nodejs/diagnostics Does it matter if the async resource type FSREQWRAP is now FSREQCALLBACK?

@mmarchini
Copy link
Contributor

I understand the reason behind this, but doesn't that mean we'll need to change the name of every wrapper once its API get a promise interface? If that's the case, we might as well change everything at once to avoid multiple breaking changes (the changes to async_hooks are semver-major IMO).

@maclover7
Copy link
Contributor Author

I understand the reason behind this, but doesn't that mean we'll need to change the name of every wrapper once its API get a promise interface?

@mmarchini Unfortunately, it depends. For example, the PR to add the promisified dns module did not require any changes to the C++ binding layer to support promises due to how the binding was originally implemented, but fs did. It seems like based on how things stand at the binding layer, this will have to be on a case-by-case basis.

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 26, 2018
mcollina
mcollina approved these changes Jul 26, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joyeecheung
Copy link
Member

FWIW, putting WRAP, PROMISE or CALLBACK in the types already seem to be leaking too many implementation details, having downstream consumers depend on these details may not be a good idea in terms of maintainability.

@Flarna
Copy link
Member

Flarna commented Jul 26, 2018

The doc in async hooks states "The type is a string identifying the type of resource that caused init to be called. Generally, it will correspond to the name of the resource's constructor.".

Based on this we have to leak implementation details. On the other hand there is no detailed specification which async resources exist (within node core) and what exactly they represent.

I would also prefer to have a cleaner separation of implementation details and async hook resource names.

As async hooks are still experimental such an API change should be currently not semver major to my understanding - but will be once they are no longer experimental.

@jasnell
Copy link
Member

jasnell commented Jul 27, 2018

Not technically semver-major, no, but async_hooks are starting to be used extensively so we should still be careful and considerate about such changes.

@addaleax
Copy link
Member

Uh … can we split this into a backportable part and a semver-major change for the async_hooks identifier? Otherwise this would be a wonderful source of merge conflicts…

@maclover7
src,lib: rename FSReqWrap to FSReqCallback
8ae3ad4 
Given that FSReqPromise does not inherit from FSReqWrap, FSReqWrap
should be renamed FSReqCallback to better describe what it does.

First of a few upcoming `fs` refactorings :)
@maclover7
src: rename PROVIDER_FSREQWRAP to PROVIDER_FSREQCALLBACK
360a102
@maclover7
Copy link
Contributor Author

Updated @addaleax (note to self/future backporters -- the second commit is the semver-major one :))

@maclover7
Copy link
Contributor Author

Full CI: https://ci.nodejs.org/job/node-test-pull-request/16042/

@maclover7
Copy link
Contributor Author

Resumed CI: https://ci.nodejs.org/job/node-test-pull-request/16122/

@maclover7
Copy link
Contributor Author

Landed in 27a5338...f479050, thank you for the reviews!

@maclover7 maclover7 closed this Aug 1, 2018
@maclover7 maclover7 deleted the jm-fs-req branch August 1, 2018 19:07
maclover7 added a commit that referenced this pull request Aug 1, 2018
@maclover7
src,lib: rename FSReqWrap to FSReqCallback
172b4d7 
Given that FSReqPromise does not inherit from FSReqWrap, FSReqWrap
should be renamed FSReqCallback to better describe what it does.

First of a few upcoming `fs` refactorings :)

PR-URL: #21971
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
maclover7 added a commit that referenced this pull request Aug 1, 2018
@maclover7
src: rename PROVIDER_FSREQWRAP to PROVIDER_FSREQCALLBACK
f479050 
PR-URL: #21971
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Flarna Flarna mentioned this pull request Oct 31, 2018
Closed
2 tasks
pull bot pushed a commit to SimenB/node that referenced this pull request Nov 2, 2018
@Flarna @vsemozhetbyt
doc: correct async_hooks resource names
7181531 
Correct async hooks resource names to match the implementation:
`FSREQWRAP` => `FSREQCALLBACK`
`TCPSERVER` => `TCPSERVERWRAP`

PR-URL: nodejs#24001
Refs: nodejs#21971
Refs: nodejs#17157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Nov 3, 2018
@Flarna @targos
doc: correct async_hooks resource names
d729f3e 
Correct async hooks resource names to match the implementation:
`FSREQWRAP` => `FSREQCALLBACK`
`TCPSERVER` => `TCPSERVERWRAP`

PR-URL: #24001
Refs: #21971
Refs: #17157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2018
@Flarna @MylesBorins
doc: correct async_hooks resource names
57ccaf6 
Correct async hooks resource names to match the implementation:
`TCPSERVER` => `TCPSERVERWRAP`

Backport-PR-URL: #24683
PR-URL: #24001
Refs: #21971
Refs: #17157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
@Flarna @MylesBorins
doc: correct async_hooks resource names
4209e12 
Correct async hooks resource names to match the implementation:
`TCPSERVER` => `TCPSERVERWRAP`

Backport-PR-URL: #24683
PR-URL: #24001
Refs: #21971
Refs: #17157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@maclover7 @nodejs-github-bot @joyeecheung @mmarchini @Flarna @jasnell @addaleax @mcollina @lpinca @cjihrig